Skip to content

Merging Orchestrator with RHDH chart #109

Merged
openshift-merge-bot[bot] merged 22 commits intoredhat-developer:mainfrom
elai-shalev:orchestrator-chart-merge
Apr 27, 2025
Merged

Merging Orchestrator with RHDH chart #109
openshift-merge-bot[bot] merged 22 commits intoredhat-developer:mainfrom
elai-shalev:orchestrator-chart-merge

Conversation

@elai-shalev
Copy link
Copy Markdown

@elai-shalev elai-shalev commented Mar 6, 2025

(WIP)
This PR will introduce changes to the RHDH helm chart, that will enable the installation of Orchestrator as a flavor of RHDH. This PR will introduce changes to the Chart Values, and will add several templates. Upon enabling orchestrator, new dynamic plugins, network policies, and other CRs will be created.

Tests and documentation are TBA.

Relates to this epic: https://issues.redhat.com/browse/RHIDP-6159

Checklist

  • Chart version bumped in Chart.yaml according to semver.
  • Variables are documented in the values.yaml and added to the README.md. The pre-commit utility can be used to generate the necessary content. Use pre-commit run -a to apply changes.
  • JSON Schema template updated and re-generated the raw schema via pre-commit hook.
  • List tests pass for Chart using the Chart Testing tool and the ct lint command.

Comment thread charts/backstage/values.yaml Outdated
Comment thread charts/backstage/README.md Outdated
Comment thread charts/backstage/README.md Outdated
Comment thread charts/backstage/README.md Outdated
Comment thread charts/backstage/README.md Outdated
Comment thread charts/backstage/README.md Outdated
Comment thread charts/backstage/templates/knatives.yaml Outdated
Comment thread charts/backstage/templates/network-policies.yaml Outdated
Comment thread charts/backstage/templates/network-policies.yaml Outdated
Comment thread charts/backstage/templates/network-policies.yaml
Comment thread charts/backstage/templates/sonataflows.yaml Outdated
@elai-shalev
Copy link
Copy Markdown
Author

@masayag Updated PR
not sure what to do with the postgresql configuration yet

@rm3l
Copy link
Copy Markdown
Member

rm3l commented Mar 11, 2025

/cc

@openshift-ci openshift-ci Bot requested a review from rm3l March 11, 2025 15:07
@rm3l
Copy link
Copy Markdown
Member

rm3l commented Mar 11, 2025

FYI

/cc @durandom

@openshift-ci openshift-ci Bot requested a review from durandom March 11, 2025 15:07
Comment thread charts/backstage/templates/dynamic-plugins-configmap.yaml Outdated
@elai-shalev elai-shalev force-pushed the orchestrator-chart-merge branch from 3e33b4b to 9aa8fb4 Compare March 19, 2025 08:19
Copy link
Copy Markdown
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ElaiShalevRH A few comments below.

Comment thread charts/backstage/values.yaml Outdated
Comment thread charts/backstage/values.yaml Outdated
Comment thread charts/backstage/values.yaml Outdated
Comment thread charts/backstage/values.yaml Outdated
Comment thread charts/backstage/templates/_helpers.tpl Outdated
Comment thread charts/backstage/templates/dynamic-plugins-configmap.yaml Outdated
Comment thread charts/backstage/templates/dynamic-plugins-configmap.yaml Outdated
Comment thread charts/backstage/templates/knatives.yaml Outdated
Comment thread charts/backstage/templates/network-policies.yaml Outdated
Comment thread charts/backstage/values.yaml Outdated
Comment thread charts/backstage/templates/knatives.yaml Outdated
@elai-shalev elai-shalev force-pushed the orchestrator-chart-merge branch from 9aa8fb4 to 3f2d1df Compare April 3, 2025 08:52
@elai-shalev elai-shalev requested a review from a team as a code owner April 3, 2025 08:52
@elai-shalev
Copy link
Copy Markdown
Author

Hey @rm3l,
I've pushed some changes after the review and after Finishing the Knative resources move to the infra chart.
I'm still having issues with getting the postgres running, as those pods are failing right now.
I'll try using the same postgresql db for rhdh and the sonataflow (OSL) resources.

Comment thread charts/backstage/Chart.yaml Outdated
Comment thread charts/backstage/README.md.gotmpl Outdated
@elai-shalev elai-shalev force-pushed the orchestrator-chart-merge branch from 3f2d1df to 767e884 Compare April 16, 2025 08:59
@elai-shalev
Copy link
Copy Markdown
Author

Hey @rm3l @masayag
Some major changes to the PR.
Now the backstage chart can be used to install orchestrator, given a pre-install of the infra chart :)
I need to test some more scenarios (multiple installations, delete) and I need to think of tests to add
but it could use a review for now

Comment thread charts/backstage/README.md
Comment thread charts/backstage/README.md Outdated
Comment thread charts/backstage/README.md Outdated
Comment thread charts/backstage/templates/_partials.tpl
Comment thread charts/backstage/templates/_partials.tpl Outdated
Comment thread charts/backstage/templates/sonataflows.yaml Outdated
Comment thread charts/backstage/templates/sonataflows.yaml
Comment thread charts/backstage/values.yaml Outdated
Comment thread charts/backstage/README.md Outdated
Comment thread charts/backstage/values.yaml Outdated
Comment thread charts/backstage/Chart.yaml Outdated
Comment thread charts/backstage/values.yaml Outdated
Comment thread charts/orchestrator-infra/templates/NOTES.txt
Comment thread charts/backstage/values.yaml Outdated
apiVersion: sonataflow.org/v1alpha08
kind: SonataFlowPlatform
metadata:
name: sonataflow-platform
Copy link
Copy Markdown
Member

@rm3l rm3l Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QQ about this: as I understand it, it is not possible to have 2 SonataFlowPlatform CRs in the same namespace (I noticed a Duplicate error in the second CR status), right?
So this means that it won't be possible to run 2 RHDH Orchestrator instances in the same namespace:

Release "my-backstage-orchestrator-2" does not exist. Installing it now.
Error: rendered manifests contain a resource that already exists.
Unable to continue with install: SonataFlowPlatform "sonataflow-platform" in namespace "my-ns" exists and cannot be imported into the current release: invalid ownership metadata;
annotation validation error: key "meta.helm.sh/release-name" must equal "my-backstage-orchestrator-2": 
current value is "my-backstage-orchestrator-1"

The problem is that we've had some (valid IMO) use cases where the user had access to a single namespace and deployed 2 instances of RHDH with different version/settings. And I can imagine the same for the Orchestrator flavor.
If this is something that you already aware of, I guess this should be added as a noteworthy note in the README.
(Or, instead of letting Helm fail, maybe check if the resource already exists and skip its creation? In which case, there might be other questions, like what happens to the SonataFlowPlatform CR if I uninstall the first Release but keep the second one?)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So a couple of things:

  1. Yes, only one sonataflowplatform can exist in each namespace, even if it's under a different name. I'll add a check to validate if it exists so the Duplicate resource won't be created.
  2. I believe multiple instanced of backstage in the same namespace, using the same sonataflowplatform should be OK, as long as the backstage versions are the same. The reason for that is that sonataflowplatform is also versioned, and the orchestrator plugin needs compatibillity between the sonataflow version and the rhdh version. an rhdh deployment using a mis-matches SFP version or OSL version will definitely create issues. So different value-backstage is OK but not different version, I think.

Not sure if we should prohibit this behavior or just not support it

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After playing with it some more, removing the first helm release will destroy the sonataflow resources for the other orchestrator instances that exist, and that is a problem...
I'm not sure how we can actually support multiple orchestrator installations in the same namespace

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think it is fine to call this out in the docs as a known limitation and leave it as it is for now. But instead of the current error from Helm which is not that clear at first sight, I would suggest, if not too much additional work, returning a better error message letting the user know about this unsupported scenario.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning an error upon a second SFP apply sounds good, though the users can always hack and simply create more rhdh+rchestrator instances, then reconfigure the SFP to point to whatever secret they want. Anyway, will document the recommended way.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take this with sonataflow team and see if we can file RFE to support more than a single platform per namespace.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rm3l is the requirement for having few RHDH instances in the same namespace a production use-case or for testing?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is something we should recommend in production or not. I've seen this so far in a discussion with one user, where they were deploying two versions of the Helm Chart in their namespace.
And this is a scenario I usually tend to also check on Dev Sandbox for example where I'm limited to a single namespace.
So it most probably makes sense for testing.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think we should even consider it for production :) , even for testing it is not really clear for testing of what user would need several instances of RHDH in one namespace?
I do not think we offer any multi-RHDH-instance scenario?

Comment thread charts/backstage/templates/network-policies.yaml
@elai-shalev
Copy link
Copy Markdown
Author

@rm3l, Moti and I had another issue I wanted to raise.
The sonataflowplatform usually requires a database, (e.g 'sonataflow') to use, for the workflow etc.
As we are using the psql instance created by rhdh, we can either use the default database 'postgres', or create a new one.

Creating a new one will require some extra steps. The fedora/postgres image does not allow init scripts or additional databases at startup, so a job is needed to exec into the psql intstance and create the new db. This will include helm templates for the job, role binding etc.

Using the default db is simple, but might cause issues down the road as it really should be seperated.

What do you think?

@rm3l
Copy link
Copy Markdown
Member

rm3l commented Apr 22, 2025

@rm3l, Moti and I had another issue I wanted to raise. The sonataflowplatform usually requires a database, (e.g 'sonataflow') to use, for the workflow etc. As we are using the psql instance created by rhdh, we can either use the default database 'postgres', or create a new one.

Creating a new one will require some extra steps. The fedora/postgres image does not allow init scripts or additional databases at startup, so a job is needed to exec into the psql intstance and create the new db. This will include helm templates for the job, role binding etc.

Using the default db is simple, but might cause issues down the road as it really should be seperated.

What do you think?

Indeed, I'd find it safer to create a new one. But why would you need additional role bindings? A Job that has a psql client and runs psql -h $host -c "CREATE DATABASE ..." could be enough, no?
Anyway, bear in mind that RHDH currently only recommends external databases for production.

@elai-shalev
Copy link
Copy Markdown
Author

Hey @rm3l
I've pushed the changes to add the logic to create the new sonataflow database with a helm job, and conditionally use local psql secret / external one.
I've also added an error for creating multiple SFP

Comment thread charts/backstage/templates/sonataflows.yaml
Comment thread charts/backstage/templates/sonataflows.yaml Outdated
Comment thread charts/backstage/templates/sonataflows.yaml Outdated
@elai-shalev elai-shalev force-pushed the orchestrator-chart-merge branch from d322093 to 8053637 Compare April 27, 2025 08:23
@sonarqubecloud
Copy link
Copy Markdown

@elai-shalev
Copy link
Copy Markdown
Author

Hey @rm3l, thanks for the review and the testing. I've added your suggestions and the CI is green now.
Ready to merge from my end, let me know if there is anything else to do :)

@elai-shalev elai-shalev changed the title (WIP) Merging Orchestrator with RHDH chart Merging Orchestrator with RHDH chart Apr 27, 2025
Copy link
Copy Markdown
Member

@rm3l rm3l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci Bot added the lgtm label Apr 27, 2025
@openshift-merge-bot openshift-merge-bot Bot merged commit ea0243d into redhat-developer:main Apr 27, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants